-
Notifications
You must be signed in to change notification settings - Fork 211
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Servant Swagger QuickCheck Property Testing (validateEveryJSON & validateEveryPath) #108
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this PR strikes a nice balance between what is possible and what is practical. We get the benefit of verifying some important properties, without writing a complete Swagger->Servant generator.
I guess at some point we may want to add another test, namely that:
"every path that exists in the Swagger specification also exists as a path in our Servant API."
However, I think it's obvious that such a test will fail unless we really do have a complete API. Therefore, we can probably wait until we do have a complete API before adding such a test.
There are some minor points in my review that hopefully could be addressed, but otherwise I think this is good. 👍
("can perform roundtrip JSON serialization & deserialization, " <> | ||
"and match existing golden files") $ do | ||
"can perform roundtrip JSON serialization & deserialization, \ | ||
\and match existing golden files" $ do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
- Parsing the specification file (which is embedded at compile-time) | ||
- Creating missing 'ToSchema' by doing lookups in that global schema | ||
|
||
2/ The above verification is rather week, because it just controls the return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above verification is rather week
Small typo, should be:
The above verification is rather weak
Just schema -> | ||
return $ NamedSchema (Just "Wallet") schema | ||
|
||
-- | Verify that every servant endpoints are present and match the specification |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verify that every servant endpoints are present and match the specification
Minor point, should be:
Verify that all servant endpoints are present and match the specification
The trick is for the later point. In a "classic" scenario, we would have | ||
defined the `ToSchema` instances directly in Haskell on our types, which | ||
eventually becomes a real pain to maintain. Instead, we have written the | ||
spec by hand, and we want to control that our implementation matches it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and we want to control that our implementation matches it.
Recommend changing the wording to:
and we want to check that our implementation matches it.
|
||
2/ The above verification is rather week, because it just controls the return | ||
types of endpoints, but not that those endpoints are somewhat valid. Thus, | ||
we've also built another check 'validateEveryPath' which crawl our servant |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
crawl
should be
crawls
2/ The above verification is rather week, because it just controls the return | ||
types of endpoints, but not that those endpoints are somewhat valid. Thus, | ||
we've also built another check 'validateEveryPath' which crawl our servant | ||
API type, and check whether every path we have in our API appears in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check
should be
checks
- construct the corresponding path (with verb) | ||
- build an HSpec scenario which checks whether the path is present | ||
This seemingly means that the identifiers we use in our servant paths (in | ||
particular, those for path parameters) should exactly match the specs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess at some point we may want to add another test, namely that:
"every path that exists in the Swagger specification also exists as a path in our Servant API."
But such a test will fail unless we really do have a complete API, so we can only add it when we do have a complete API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Thought of it indeed but in practice, we will likely modify and extend the specification always before adding the corresponding implementation. We could also just have "stubs" in the Haskell with empty handlers throwing 501. It could work quite fine without much overhead when modifying specs.
2446e67
to
904c023
Compare
@jonathanknowles Wording & Grammar (& line formatting) reviewed 👍 |
904c023
to
7fcba48
Compare
226a37b
to
df92987
Compare
Addressed gramatical & styling points in: https://github.com/input-output-hk/cardano-wallet/compare/2446e6779f87586a2fa016b50122e90551ac1fc6..904c023325f0f0f9bd8bb379b198a2face7e2741
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
src/Cardano/Wallet/Api/Types.hs
Outdated
@@ -67,13 +67,13 @@ import qualified Data.Aeson.Types as Aeson | |||
-------------------------------------------------------------------------------} | |||
|
|||
data Wallet = Wallet | |||
{ _id :: !WalletId | |||
{ _id :: !(ApiT WalletId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason we are wrapping all in ApiT
that I am not aware off?
Or we explicitly want to keel all json instances defined in this module?
aha - we probably want to enforce the same genericToJSON
and genericParseJSON
rules for the whole api, so we are going to wrap most/all things with ApiT
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-> Or we explicitly want to keel all json instances defined in this module?
You said it :)
A nice explanation from @jonathanknowles here: #107 (comment)
df92987
to
d0ae386
Compare
7fcba48
to
7a06db9
Compare
roundtripAndGolden $ Proxy @ Wallet | ||
roundtripAndGolden $ Proxy @ (ApiT AddressPoolGap) | ||
roundtripAndGolden $ Proxy @ (ApiT WalletBalance) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate with a line below
…s we generate are actually compliant with the spec
…ance with the API specification
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉Awesome!
Observation: noting that we're (obviously) not checking that everything in the api-spec exists in our API-type — ah which you essentially also mentioned
roundtripAndGolden $ Proxy @ (ApiT (WalletDelegation (ApiT PoolId))) | ||
roundtripAndGolden $ Proxy @ (ApiT WalletId) | ||
roundtripAndGolden $ Proxy @ (ApiT WalletName) | ||
roundtripAndGolden $ Proxy @ (ApiT WalletBalance) | ||
roundtripAndGolden $ Proxy @ (ApiT WalletPassphraseInfo) | ||
roundtripAndGolden $ Proxy @ (ApiT WalletState) | ||
|
||
-- | Run JSON roundtrip & golden tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove this line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 Good question? Probably merge-conflict resolution that went little wrong here.
# # | ||
############################################################################# | ||
|
||
address: &address |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am assuming you have just relocated and polished a few things here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! I actually forgot about this while writing the spec file but, Swagger expects "definitions" of model / resources to be located in a "definitions" properties from the root swagger file. So I just moved things to be located under this field.
7a06db9
to
25d5c9e
Compare
Issue Number
#53
Overview
I have added a serie of property based tests to verify that every type used with JSON content type in a servant API has compatible ToJSON and ToSchema instances.
I have added a serie of property based tests to verify that every path specified by the servant server matches an existing path in the specification
Comments
see 'validateEveryToJSON' test case failure
If one of them is failing, it is reported as:
If the specification document is missing the code won't even compile and fail with a rather descriptive message.
If the specification document can't be parsed correctly, because the yaml is invalid, the tests will fail.